Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix OOB array index in circle segment calculation #5317

Closed
wants to merge 1 commit into from
Closed

fix OOB array index in circle segment calculation #5317

wants to merge 1 commit into from

Conversation

jdpatdiscord
Copy link

Version/Branch of Dear ImGui: master

Back-end/Renderer/Compiler/OS:
Windows, Dx11

My Issue/Question:
A few months ago in production this crash happened to our users as an extremely rare edge case.
The crash ceased to occur when adding the bounds check before the array is indexed.

Screenshots/Video:
I did not archive anything from that time.

Standalone, minimal, complete and verifiable example:
Call _CalcCircleAutoSegmentCount with something negative and watch what happens.

P.S. I did not go and try to see if this function receiving a negative value was a side effect of another part of the codebase. I did what was necessary to fix the crash and did not observe any consequences of adding the bounds check.

@ocornut
Copy link
Owner

ocornut commented May 16, 2022

Hello,
This has actually been fixed last friday with 9e0517a (see #5249, #5293)

@ocornut
Copy link
Owner

ocornut commented May 16, 2022

This has actually been fixed last friday with 9e0517a (see #5249, #5293)

Actually not exactly. We fixed something very close to that, and I would say it is extremely likely that your case would be fixed by our change, but I'm not 100% sure since they are done at different level of code.
I cannot merge an extra random bound check without an understanding of what would repro it.

@jdpatdiscord
Copy link
Author

jdpatdiscord commented May 16, 2022

Sorry, I do not recall the exact scenario it happened with. Somehow it got called with a negative value. It would be good for this to be fixed regardless, as it did happen in normal usage. The end users were calling an abstraction via Lua bind.

I noticed some of those bounds checks up the call stack and I'm unaware if those had existed back a few months ago. It could've fixed the issue but I am now unable to reproduce it.

If you really think the bounds check on this function is unnecessary, then sure... but make it more verbose that this never receives a negative value.

@ocornut
Copy link
Owner

ocornut commented May 16, 2022

In the current code it would make a difference if private/internal _CalcCircleAutoSegmentCount() function is called with a negative radius. Looking at the 3 call sites I don't see how that's possible so IHMO there's no bug right now (unless _CalcCircleAutoSegmentCount() is called directly, which is technically illegal as it is a private API).

What puzzle me is how precisely the PR you are suggesting would fix anything even in the old version. Your fix protects against NEGATIVE radiuses but all calls to _CalcCircleAutoSegmentCount() already had a radius <= 0 check for that. My guess is that the crash you had was not an OOB but the divide-by-zero when pulling from CircleSegmentCounts[0], bug outlined and fixed in #5249. Your fix didn't actually fix that, maybe but it's just that you experienced the crash rarely enough you thought it was fixed. If you trace the code prior to your patch the call sites make it pretty obvious. If your fix used radius_idx > 0 I understand how it would make a difference, but I can't see a difference being made with radius_idx >= 0.

@ocornut ocornut closed this May 16, 2022
ocornut pushed a commit that referenced this pull request Aug 6, 2023
@ocornut
Copy link
Owner

ocornut commented Aug 6, 2023

Turns out this could repro which an excessively large floating point value (e.g FLT_MAX or even e.g. 100000000.0f) which would break during the downcast to int. See #6657. Merged same fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants